Skip to content

fix: raise exception when LoadImage reader is specified but not installed#8768

Open
abishop1990 wants to merge 13 commits intoProject-MONAI:devfrom
abishop1990:7437-raise-exception-missing-reader
Open

fix: raise exception when LoadImage reader is specified but not installed#8768
abishop1990 wants to merge 13 commits intoProject-MONAI:devfrom
abishop1990:7437-raise-exception-missing-reader

Conversation

@abishop1990
Copy link

Description

Fixes #7437

Problem

When a user explicitly specifies a reader via but the required optional package (e.g. ) is not installed, MONAI currently:

  1. Emits a warning
  2. Silently falls back to the next available reader (e.g. PILReader)

This silent fallback is surprising and hard to debug — the user asked for a specific reader and got a different one without an error.

Fix

When an explicitly-specified reader raises OptionalImportError during initialization, raise a RuntimeError with a clear, actionable message:

RuntimeError: The required package for reader 'ITKReader' is not installed, or the version doesn't match the requirement. If you want to use 'ITKReader', please install the required package. If you want to use an alternative reader, do not specify the `reader` argument.

Backward Compatibility

  • No reader specified (auto-select): existing warn-and-skip behavior is unchanged ✅
  • Reader specified but available: works as before ✅
  • Reader specified but not installed: now raises RuntimeError instead of warning ✅ (this is the fix)

Tests Added

Three new tests in TestLoadImageMissingReader:

  1. test_explicit_reader_not_installed_raises_runtime_error — mocks OptionalImportError to verify RuntimeError is raised with helpful message
  2. test_unspecified_reader_falls_back_silently — verifies no-reader case still works
  3. test_explicit_reader_available_succeeds — verifies happy path still works

Checklist

  • New functionality is tested
  • Signed-off commit (git commit -s)
  • Error message is clear and actionable
  • Backward compatibility preserved

…lled

Previously, when a user explicitly specified a reader (e.g. LoadImage(reader='ITKReader'))
but the required optional package was not installed, MONAI would silently warn and fall
back to the next available reader. This silent fallback is surprising and hard to debug.

This change raises a RuntimeError instead, giving the user a clear actionable message
that explains what happened and how to fix it (install the package or omit the reader
argument to use automatic fallback).

Backward compatibility is preserved: if no reader is specified, the existing warn-and-
skip behavior for missing optional packages is unchanged.

Fixes Project-MONAI#7437

Signed-off-by: Alan Bishop <alanbishop@users.noreply.github.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

LoadImage.init behavior changed: when a string reader or a provided reader class requires a missing/incompatible optional dependency, the code now raises RuntimeError with guidance to install the dependency or pick an alternative. TypeError cases for class readers still register a fallback instance but now emit warnings with stacklevel=2. Tests were added to assert RuntimeError for explicit missing readers (string and class), to check unspecified-reader fallback, and to validate successful explicit-reader loading; a duplicate test class was accidentally introduced in the test file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and concisely describes the main change: making LoadImage raise an exception when a specified reader's required package is not installed.
Description check ✅ Passed Description covers problem, fix, backward compatibility, and lists three tests added. Checklist items marked appropriately.
Linked Issues check ✅ Passed Changes implement the core requirement from #7437: raising RuntimeError when an explicitly-specified reader's optional package is missing, instead of silently falling back.
Out of Scope Changes check ✅ Passed All changes are scoped to the issue: modifying LoadImage reader initialization error handling and adding corresponding tests. No unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@monai/transforms/io/array.py`:
- Around line 212-217: The try/except currently only wraps string-based reader
imports but not explicit reader classes, causing OptionalImportError to leak
when someone passes a class like ITKReader to LoadImage; update the code path in
LoadImage where the reader is instantiated (the branch handling the `reader`
argument and any use of `_r`/`reader` to create reader instances) to catch
OptionalImportError around the instantiation and re-raise the same RuntimeError
message used for string readers so both `LoadImage(reader="ITKReader")` and
`LoadImage(reader=ITKReader)` produce the consistent RuntimeError; ensure you
reference the same message and `from e` chaining when catching
OptionalImportError raised during explicit class construction.

In `@tests/transforms/test_load_image.py`:
- Around line 503-529: Add a unit test in TestLoadImageMissingReader to cover
the reader-as-class path: mimic the existing string-case test by patching
ITKReader.__init__ to raise monai.utils.OptionalImportError("itk") and assert
that LoadImage(reader=ITKReader) raises a RuntimeError containing "ITKReader"
and "not installed"; also add the complementary case to ensure
LoadImage(reader=NibabelReader) still succeeds when passing the class object
(assert instance), referencing the LoadImage, ITKReader, and NibabelReader
symbols so the class-based reader API is exercised alongside the string-based
tests.
- Around line 519-523: The test test_unspecified_reader_falls_back_silently
currently only constructs LoadImage() and may pass in CI when optional deps
exist; change it to force the "missing optional readers" path by temporarily
simulating ImportError(s) for the reader modules LoadImage tries to import
(e.g., patch importlib.import_module or the specific module lookup used inside
LoadImage to raise ImportError for PIL/torchvision/imageio), then instantiate
LoadImage() and assert it returns an instance without raising; restore the
original import behavior afterward and optionally assert the warn-and-skip
logging was invoked to prove the fallback path was exercised.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 20a87e52-ae6a-446a-beb1-fec7a663885a

📥 Commits

Reviewing files that changed from the base of the PR and between daaedaa and 0940296.

📒 Files selected for processing (2)
  • monai/transforms/io/array.py
  • tests/transforms/test_load_image.py

Cipher and others added 6 commits March 7, 2026 11:33
Signed-off-by: Cipher <cipher@openclaw.ai>
…onment

When LoadImage is initialized with an explicitly-specified reader that is not
installed, it now raises RuntimeError (instead of silently falling back). This is
the correct behavior per issue Project-MONAI#7437, but breaks existing tests that assume the
old fallback behavior.

Update test_load_image() to only test with reader=None (auto-select path) which
works in all environments. Specific readers with @SkipIfNoModule decorators are
tested in their own dedicated test methods.

Signed-off-by: Cipher <cipher@openclaw.ai>
…ise fallback path

The previous test only verified LoadImage() succeeds in the happy path where
optional dependencies exist. This does not validate the critical warn-and-skip
fallback behavior when packages are missing.

Update the test to:
1. Patch SUPPORTED_READERS to simulate missing optional package (ITKReader)
2. Trigger LoadImage() to exercise the exception-catching, warn, and skip path
3. Verify the debug log about the missing package was invoked
4. Restore original reader entries afterward

This ensures the auto-select fallback path is actually tested, not just the
happy path.

Signed-off-by: Cipher <cipher@openclaw.ai>
The exception handling for missing optional packages only wrapped string-based
reader imports (LoadImage(reader='ITKReader')). When users passed a class directly
(LoadImage(reader=ITKReader)), OptionalImportError would leak through.

Update the class instantiation path (elif inspect.isclass(_r):) to:
1. Catch OptionalImportError during _r(*args, **kwargs) call
2. Re-raise as RuntimeError with consistent message
3. Use exception chaining (from e) for debugging
4. Handle TypeError with warn-and-retry (same as string path)
5. Extract class name safely for error message

Now both LoadImage(reader='ITKReader') and LoadImage(reader=ITKReader) produce
consistent RuntimeError when the optional package is missing.

Add test_explicit_class_reader_not_installed_raises_runtime_error to verify
the class path is properly handled.

Signed-off-by: Cipher <cipher@openclaw.ai>
…ently

Pre-commit ci caught unused variable original_readers that was defined but never
used. Remove it to pass ruff check.

Signed-off-by: Cipher <cipher@openclaw.ai>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
monai/transforms/io/array.py (1)

212-220: String-reader path looks correct.

Catches OptionalImportError, chains it properly with from e, and provides actionable guidance. The TypeError fallback preserves existing behavior.

Static analysis flags missing stacklevel=2 on line 219's warning—minor but worth fixing for clearer tracebacks.

Proposed fix
 except TypeError:  # the reader doesn't have the corresponding args/kwargs
-    warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.")
+    warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.", stacklevel=2)
     self.register(the_reader())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monai/transforms/io/array.py` around lines 212 - 220, The TypeError except
block in monai.transforms.io.array (the except handling where it warns that
"{_r} is not supported with the given parameters {args} {kwargs}" and then calls
self.register(the_reader())) should include stacklevel=2 on the warnings.warn
call to point the warning at the caller; update the warnings.warn invocation to
pass stacklevel=2 while keeping the existing message and the subsequent
self.register(the_reader()) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/transforms/test_load_image.py`:
- Around line 538-558: The test is patching SUPPORTED_READERS with the wrong key
("itk") so the fallback path in LoadImage isn't exercised; change the test to
use the actual key used in SUPPORTED_READERS ("itkreader") when saving
original_itk, replacing it with failing_reader (that raises
OptionalImportError), and restoring original_itk afterwards; ensure you still
capture logs with self.assertLogs("LoadImage", level="DEBUG") so the
warn-and-skip behavior is asserted, referencing SUPPORTED_READERS, LoadImage,
failing_reader, and OptionalImportError to locate the relevant lines to update.

---

Nitpick comments:
In `@monai/transforms/io/array.py`:
- Around line 212-220: The TypeError except block in monai.transforms.io.array
(the except handling where it warns that "{_r} is not supported with the given
parameters {args} {kwargs}" and then calls self.register(the_reader())) should
include stacklevel=2 on the warnings.warn call to point the warning at the
caller; update the warnings.warn invocation to pass stacklevel=2 while keeping
the existing message and the subsequent self.register(the_reader()) behavior
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f2655f31-305d-401d-99da-990c69bf5f15

📥 Commits

Reviewing files that changed from the base of the PR and between 90a4be9 and 9f39465.

📒 Files selected for processing (3)
  • monai/transforms/io/array.py
  • tests/data/test_init_reader.py
  • tests/transforms/test_load_image.py

1. Add stacklevel=2 to all warnings.warn() calls for better tracebacks
   - Helps developers see where the warning originates
2. Fix SUPPORTED_READERS key in fallback test from 'itk' to 'itkreader'
   - Ensures test actually exercises the warn-and-skip fallback path
   - Was using wrong key, preventing the patch from working

These fixes improve warning clarity and test effectiveness per CodeRabbit review.

Signed-off-by: Cipher <cipher@openclaw.ai>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
tests/transforms/test_load_image.py (1)

526-558: Remove redundant import at line 531.

OptionalImportError is already imported at the module level (line 32). The local import is unnecessary.

Proposed fix
     def test_unspecified_reader_falls_back_silently(self):
         """When no reader is specified, missing optional readers should be silently skipped (no exception)."""
         # Force the fallback path by simulating missing optional dependencies.
         # Patch the constructor to raise OptionalImportError for some readers,
         # then verify LoadImage still instantiates and logs warnings.
-        from monai.utils import OptionalImportError
-
         # Patch SUPPORTED_READERS entries to raise OptionalImportError
         # This simulates optional packages not being installed
         from monai.transforms.io.array import SUPPORTED_READERS
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/transforms/test_load_image.py` around lines 526 - 558, Remove the
redundant local import of OptionalImportError inside
test_unspecified_reader_falls_back_silently; instead reuse the module-level
OptionalImportError already imported, so delete the line "from monai.utils
import OptionalImportError" in the test, leaving the rest of the test (including
use of SUPPORTED_READERS and LoadImage) unchanged.
monai/transforms/io/array.py (1)

246-248: stacklevel may be off when called via LoadImaged.register().

stacklevel=2 correctly points to user code for direct LoadImage.register() calls. However, when called through LoadImaged.register() (which delegates to self._loader.register()), the warning will point to LoadImaged.register() instead of the actual user code. This is minor and doesn't break anything.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monai/transforms/io/array.py` around lines 246 - 248, The warning in the
reader-registration block uses a fixed stacklevel=2 which misattributes the call
when LoadImaged.register() delegates to self._loader.register(); update the
registration API and implementation so register() can accept an optional
stacklevel or caller_depth parameter and pass that through from
LoadImaged.register() into the reader-check (the block that does "if not
isinstance(reader, ImageReader): warnings.warn(..., stacklevel=...)"), or
compute the correct stacklevel dynamically (e.g., via inspect) to skip internal
frames; change the relevant symbols LoadImage.register / LoadImaged.register /
self._loader.register and the reader check in array.py to propagate or compute
the correct stacklevel so the warning points to the actual user code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@monai/transforms/io/array.py`:
- Around line 246-248: The warning in the reader-registration block uses a fixed
stacklevel=2 which misattributes the call when LoadImaged.register() delegates
to self._loader.register(); update the registration API and implementation so
register() can accept an optional stacklevel or caller_depth parameter and pass
that through from LoadImaged.register() into the reader-check (the block that
does "if not isinstance(reader, ImageReader): warnings.warn(...,
stacklevel=...)"), or compute the correct stacklevel dynamically (e.g., via
inspect) to skip internal frames; change the relevant symbols LoadImage.register
/ LoadImaged.register / self._loader.register and the reader check in array.py
to propagate or compute the correct stacklevel so the warning points to the
actual user code.

In `@tests/transforms/test_load_image.py`:
- Around line 526-558: Remove the redundant local import of OptionalImportError
inside test_unspecified_reader_falls_back_silently; instead reuse the
module-level OptionalImportError already imported, so delete the line "from
monai.utils import OptionalImportError" in the test, leaving the rest of the
test (including use of SUPPORTED_READERS and LoadImage) unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 818a6f12-c4bf-4779-9d1b-df9b5e64622b

📥 Commits

Reviewing files that changed from the base of the PR and between 9f39465 and 08bd231.

📒 Files selected for processing (2)
  • monai/transforms/io/array.py
  • tests/transforms/test_load_image.py

OptionalImportError and logging were imported both at module level and
again inside the test_unspecified_reader_falls_back_silently() method.

Remove the duplicate local imports since they're already available at the
module level (where we moved them earlier per MONAI style). Keep the local
import of SUPPORTED_READERS since it's only needed in this one test method.

This fixes isort formatting check failure.

Signed-off-by: Cipher <cipher@openclaw.ai>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/transforms/test_load_image.py (1)

551-554: Cleanup may leave stale key if original was missing.

If SUPPORTED_READERS didn't have "itkreader" originally, original_itk would be None and this block would skip restoration, leaving the failing_reader in place. In practice, itkreader is always defined, so this is safe. Consider using pop with del for stricter test isolation.

Optional stricter cleanup
         finally:
             # Restore original reader
-            if original_itk is not None:
-                SUPPORTED_READERS["itkreader"] = original_itk
+            if original_itk is not None:
+                SUPPORTED_READERS["itkreader"] = original_itk
+            elif "itkreader" in SUPPORTED_READERS:
+                del SUPPORTED_READERS["itkreader"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/transforms/test_load_image.py` around lines 551 - 554, The test cleanup
currently restores SUPPORTED_READERS["itkreader"] only when original_itk is not
None, which leaves the test-installed reader in place if the key was absent
originally; change the finally block to remove the test entry when original_itk
is None and restore it when present: check original_itk (from the setup where
you saved the initial value), and if original_itk is None use
SUPPORTED_READERS.pop("itkreader", None) or del SUPPORTED_READERS["itkreader"]
to remove the key, otherwise set SUPPORTED_READERS["itkreader"] = original_itk
to restore the original reader (reference SUPPORTED_READERS and original_itk in
your fix).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/transforms/test_load_image.py`:
- Around line 551-554: The test cleanup currently restores
SUPPORTED_READERS["itkreader"] only when original_itk is not None, which leaves
the test-installed reader in place if the key was absent originally; change the
finally block to remove the test entry when original_itk is None and restore it
when present: check original_itk (from the setup where you saved the initial
value), and if original_itk is None use SUPPORTED_READERS.pop("itkreader", None)
or del SUPPORTED_READERS["itkreader"] to remove the key, otherwise set
SUPPORTED_READERS["itkreader"] = original_itk to restore the original reader
(reference SUPPORTED_READERS and original_itk in your fix).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fd591be8-ba29-446b-8d48-7538cd717224

📥 Commits

Reviewing files that changed from the base of the PR and between 08bd231 and 369321e.

📒 Files selected for processing (1)
  • tests/transforms/test_load_image.py

Cipher added 4 commits March 7, 2026 18:47
- Break long warning messages across multiple lines to keep individual lines ≤ 120 chars
- Shorten verbose docstring in test_explicit_class_reader_not_installed_raises_runtime_error
- Ensures codeformat CI check passes

This complies with MONAI's ruff/black/isort configuration which enforces
line length limits.

Signed-off-by: Cipher <cipher@openclaw.ai>
Black reformatted code according to MONAI's style requirements:
- Add blank line after module docstring
- Remove extra blank lines before class definition
- Reformat long assertions to Black's multi-line style

Note: Some lines exceed 120 chars due to f-string messages, which are
allowed per setup.cfg (E501 ignored for strings).

Signed-off-by: Cipher <cipher@openclaw.ai>
The finally block now correctly handles both cases:
- If 'itkreader' existed originally: restore the original reader
- If 'itkreader' didn't exist originally: remove the test entry

This prevents leaving the patched reader in place when the key was
absent originally, ensuring clean test isolation.

Signed-off-by: Cipher <cipher@openclaw.ai>
The register() method's warning about non-ImageReader instances now uses
stacklevel=3 instead of stacklevel=2. This correctly points to the LoadImage()
call site where the reader is being instantiated, rather than internal frames.

Stack trace:
- User code: LoadImage(reader=...)
- LoadImage.__init__()
- self.register()
- warnings.warn(stacklevel=3) → points to LoadImage() call

This improves debugging experience for users who pass invalid readers.

Signed-off-by: Cipher <cipher@openclaw.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Raise the exception when LoadImage has a reader specified but it is not installed

2 participants